pallet_revive: Change EVM call opcodes to respect the gas limit passed #10018
Conversation
athei
requested review from
TorstenStueber,
pgherveou and
xermicus
and removed request for
pgherveou
|
/cmd prdoc --audience runtime_dev |
|
I could validate that this resolves paritytech/contract-issues#119 for EVM execution (but not for PVM execution). |
|
Yes PVM is not fixed, yet. It is listed as follow ups in the PR description. |
I want to run them in a separate PR once all the open stuff is merged. Otherwise we are creating too many merge conflicts. |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
Will run the benchmarks here once #9418 is merged. |
| let weight_fee_available = | ||
| T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left(), Combinator::Min); | ||
| let available_balance = self | ||
| let weight_fee_available = T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left()); |
There was a problem hiding this comment.
If we calculate weight_to_fee for gas left instead of gas consumed, we would need to combine via min instead of via max again.
There was a problem hiding this comment.
Additionally: weight_fee_available needs to be multiplied be the next fee multiplier.
| .origin | ||
| .account_id() | ||
| .map(|acc| { | ||
| T::Currency::reducible_balance(acc, Preservation::Preserve, Fortitude::Polite) |
There was a problem hiding this comment.
Why do we use the reducible_balance of the origin here? This has no meaning if exec_config is ethereum like (i.e., self.exec_config.collect_deposit_from_hold is Some).
Shouldn't we instead limit the initial storage deposit limit for all substrate like executions by initially checking that this limit is at most the origin's reducible balance – or better even, reduce the origin's balance initially by this deposit limit for substrate like transactions?
|
|
||
| // the gas_limit is in unadjusted fee | ||
| let deposit_limit = { | ||
| let weight_fee = T::FeeInfo::weight_to_fee(&weight_limit); |
There was a problem hiding this comment.
This should also be a weight_to_fee calculation where we take the minimum instead of maximum to be consistent with https://github.com/paritytech/polkadot-sdk/pull/10018/files#r2435295902
There was a problem hiding this comment.
I think that there is a general problematic inconsistency with the fact that the gas_left is the minimum of
- the available imbalance in tx payment
- the sum of a) the available gas as determined by available weight and b) the available gas as determined by available storage deposit
But both are incompatible – Example: when the first frame immediately makes a call to another contract and does not explicitly limit gas, then in calc_limits, gas_limit equals available.
Then weight_limit is equal to nested_gas.gas_left(). Since this is right at the beginning of the first frame, this is essentially the original weight limit as determined in try_into_checked_extrinsic.
By definition FeeInfo::weight_to_fee of this original weight limit is then almost equal to ((the tx payment imbalance amount) / (next fee multiplier)) [ignoring some small constant values like the FeeInfo::fixed_fee].
But "((the tx payment imbalance amount) / (next fee multiplier))" is exactly what self.gas_left() returns in calc_limits because max_by_credit_hold turns out to be (the tx payment imbalance amount) given that at this point essentially no weight or deposit has been consumed.
Thus, in calc_limits:
weight_fee- =
T::FeeInfo::weight_to_fee(&weight_limit) - =
FeeInfo::weight_to_feeof the original weight limit - = ((the tx payment imbalance amount) / (next fee multiplier))
- =
self.gas_left()
Thus, deposit_limit as returned by calc_limits will essentially be zero. Not what we want.
Proposal
I propose that we have completely different semantics for ethereum like or substrate like transactions.
For substrate like transactions both the initial weight and deposit limits can be used fully independently – keep the code as it is.
For ethereum like transactions the semantics is different: the gas limit can be either used for weight fees or for storage deposits – which one is unknown before. We should change:
- gas left = max(
FeeInfo::weight_to_fee(limit of top frame's gas meter) * (next fee multiplier), (limit of top frame's deposit meter)) - (total consumed) where- total consumed =
FeeInfo::weight_to_fee(consumed of top frame's gas meter) * (next fee multiplier) + (consumed of top frame's deposit meter)
- total consumed =
- in
calc_limit- for the case
CallResources::Ethereum- set
weight_leftto (limit of top frame's gas meter) -FeeInfo::fee_to_weight(total consumed) / (next fee multiplier) - the returned deposit limit should be ((limit of top frame's deposit meter) - (total consumed)) *
ratio
- set
- for the case
CallResources::Precise- don't return
(*weight, *deposit_limit)directly but- limit the first entry by (limit of top frame's gas meter) -
FeeInfo::fee_to_weight(total consumed) / (next fee multiplier) - limit the second entry by (limit of top frame's deposit meter) - (total consumed)
- limit the first entry by (limit of top frame's gas meter) -
- don't return
- for the case
Additionally, remove the calculation of max_by_credit_hold in gas_left, it is not required anymore.
|
I realized that my proposal also has shortcomings. Will make another proposal today. |
|
The revised model is defined here: https://shade-verse-e97.notion.site/Gas-Tracking-2928532a7ab5808381b4e688fcc58838?source=copy_link |
4 participants
So far the EVM family of call opcodes did ignore the
gasargument passed to them. The consequence was that we were not able to limit the resource usage of sub contract calls. This PR changes that. Gas is now fully functional on the EVM backend.The resources of any sub contract call are now effectively limited. This is both true for
Weightand storage deposit. The algorithm works in a way that if you passx%of the currentGASthe theCALLopcode the sub call will havex%of currently availableWeightand storage deposit available. This allows the caller to always make sure to execute code after retuning from a sub call.Changes to the gas meter
I needed to change the gas meter to track
gas_consumedinstead ofgas_left. Otherwise it is not possible to know the total amount of gas spent for a call stack that is not unwinded, yet.Followup
Weightand storage deposit limit